feat(android): background-size, position and repeat styles#52282
feat(android): background-size, position and repeat styles#52282intergalacticspacehighway wants to merge 11 commits into
Conversation
null checks
| view: View, | ||
| backgroundRepeats: List<BackgroundRepeat>? | ||
| ): Unit { | ||
| if (ReactNativeFeatureFlags.enableNewBackgroundAndBorderDrawables()) { |
There was a problem hiding this comment.
Only enabled for new drawables as it is true by default now.
| public companion object { | ||
| @JvmStatic | ||
| public fun setFromDynamic(dynamic: Dynamic): LengthPercentage? { | ||
| public fun setFromDynamic(dynamic: Dynamic, allowNegative: Boolean = false): LengthPercentage? { |
There was a problem hiding this comment.
Did not like this change, but we need to support negative values for background-position. lmk if I should take some other approach!
There was a problem hiding this comment.
I think this is fine.. I'll think about it a bit
|
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D82993837. |
jorge-cab
left a comment
There was a problem hiding this comment.
Logic and functionality seem good! My main concern is that we are bloating BackgroundDrawable quite a bit with something that should realistically be a different layer.
I think this should just be another drawable BackgroundImageDrawable and should be set between the BackgroundDrawable and BorderDrawable in the CompositeBackgroundDrawable class.
Recently we just deleted CSSBackgroundDrawable which was combining both the Background and Border layers, which took an annoying amount of time so I would like to avoid that situation as much as possible.
Specially once we start looking into adding background-image which will require even more logic and integration with an Image pipeline
| canvas.save() | ||
| // 1. create a clipping path that matches the border radius. | ||
| val clipPath = Path() | ||
| val cornerRadii = |
There was a problem hiding this comment.
Using paths to draw is slower than using drawRoundRect or drawRect which is why before we only used paths when necessary. I think this should be something you can do here?
Overall I don't think we should remove the original code, just expand another conditional for when we have size/repeat/position set, but maybe I'm missing something
There was a problem hiding this comment.
Simplified it a lot in last commits. We just need Path to clip the drawable. e.g. on background repeat: repeat we have to repeat gradients that go out of bounds, so need to clip them (on iOS we handle it using maskToBounds))
| public enum class BackgroundSizeKeyword { | ||
| Cover, | ||
| Contain | ||
| } |
There was a problem hiding this comment.
If I get this correctly we are no-oping on Cover and Contain since gradients don't use them, maybe we shouldn't create a class and complicate the logic to accomodate them?
That would also simplify BackgroundSize quite a bit and we can deal with this complexity once we actually need to implement it
There was a problem hiding this comment.
Agree. Done.
| NoRepeat | ||
| } | ||
|
|
||
| public class BackgroundRepeat( |
There was a problem hiding this comment.
I think for this and most other classes you added we should make them private unless you have a clear reason to make them public
There was a problem hiding this comment.
yeah, we need them in BackgroundStyleApplicator so need to keep them public i guess. I think most styles are public right now. e.g. OutlineStyle
| public companion object { | ||
| @JvmStatic | ||
| public fun setFromDynamic(dynamic: Dynamic): LengthPercentage? { | ||
| public fun setFromDynamic(dynamic: Dynamic, allowNegative: Boolean = false): LengthPercentage? { |
There was a problem hiding this comment.
I think this is fine.. I'll think about it a bit
|
Position also looks off in Android but in a different way than iOS (#52283 (comment)) |
|
@jorge-cab thanks for catching the issue. There was rounding pixel error on android and iOS was accounting background position for repeat space styling. I have pushed fixes on iOS and Android, also added the above example in JS PR.
Makes a lot of sense. Just added Background Image drawable and cleaned up Background drawable. Maybe later we can rename Background drawable to Background color drawable. |
jorge-cab
left a comment
There was a problem hiding this comment.
Clean! I'll have this go through internal review and will update here with any other requested changes. Thank you for working on this!
…react/uimanager/drawable/BackgroundImageDrawable.kt Co-authored-by: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com>
…react/uimanager/drawable/BackgroundImageDrawable.kt Co-authored-by: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com>
7dbf040 to
478dc99
Compare
cortinico
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D82993837. |
|
@jorge-cab we need to run e2e after merging this PR with JS PR. I checked the e2e failing artifact. It has below error, it gets fixed when we add the JS changes. Also i tested radial/linear gradients on API level 24 emulator (with the JS changes). Seems to be working for me. lmk if i am missing something!
|
Summary: This PR adds support for background size, position and repeat styles. It follows the [CSS](https://www.w3.org/TR/css-backgrounds-3/#backgrounds) spec. Currently we default to `background-origin: padding-box` and `background-clip : border-box` to match the web's behavior. We can introduce these styles later. I have split the PR intro three parts for review. This PR includes JS parsing and style propagation to native changes. I wanted to introduce one style at a time, but CSS spec is such that size, position and repeat are intertwined. ## Changelog: [GENERAL][ADDED] - Background size, position and repeat styles. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #52284 Test Plan: Merge the [iOS](#52283) and [android](#52282) PR into this, this PR includes `BackgroundImageExample`. I have also added testcases for parsing syntax in JS. https://github.com/user-attachments/assets/b7192fdf-52ba-4eb0-a1be-d47c72d87e92 Reviewed By: joevilches Differential Revision: D82973282 Pulled By: jorge-cab fbshipit-source-id: a94e33962c6708be963e1cac049da50d4764da64
|
@jorge-cab merged this pull request in e859293. |





Summary:
This PR adds support for background size, position and repeat styles. It follows the CSS spec. Currently we default to
background-origin: padding-boxandbackground-clip : border-boxto match the web's behavior. We can introduce these styles later. I have split the PR intro three parts for review. This PR includes android only changes. I wanted to introduce one style at a time, but CSS spec is such that size, position and repeat are intertwined.Changelog:
[ANDROID][ADDED] - Background size, position and repeat styles.
Test Plan:
Merge the JS PR, rebuild android app and test RNTester app, it includes
BackgroundImageExample. I have also added testcases for parsing syntax in JS.Screen.Recording.2025-06-26.at.10.47.17.AM.mp4